-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x.py: allow configuring the build directory #71994
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hmm, it looks like the repository is read-only on CI. Could we make the build directory configurable so that it defaults to |
Out of source builds worked before, they just regress regularly due to not being tested. |
I would prefer not to change the default here but would not object to exposing an option to choose the build dir. I'm not sure that a change as minimal as this PR suggests will be able to do it, though. |
What's the use case for not changing it? Right now all it does is rebuild the entire compiler when you change directories.
I pushed a change that allows configuring the build directory, I'm not sure how to pass that to CI though. I guess that if we're not changing the default it won't matter. |
If we changed the default, we'd have out-of-source builds always be tested on CI, and the normal use case of not passing a flag would be tested by every developer working on rustc. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
It looks like changing this on CI is non-trivial since it imports the |
One easy answer is "don't change directories", and it can also be seen as a feature. But OTOH it's not what cargo does I believe and maybe we should match that. I'm not sure. The main argument against doing the default change is it's somewhat likely it'll break people's workflows -- if we have an option exposed that says "I want the old behavior" (possibly via configure just creates config.toml so we should be able to mv that if needed? |
Hmm so the idea is instead of making it an argument it's set in config.toml? I don't think config.toml is parsed until the Rust code, does CI run before or after
Awesome, yeah my change would allow this with |
Ok I think I have an idea what's going on here ...
So the way I would make this configurable is, instead of adding a flag to x.py, reading a section from |
CI runs, well, I'm not sure I follow you -- bootstrap.rs is built on CI, so "both"? Okay if I believe what you suggested makes sense but I didn't think too much about it. |
My question was unclear, I meant to ask whether |
Ok, so now what happens is |
I'd still like to change the default just because I think it will be what new developers will expect but I understand that could break workflows. Maybe there's a way to ask for feedback on this somehow? It seems a shame to always be locked into the current defaults. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hmm, the failure message looks like it tried to run a Also, while debugging that I noticed that there is already an |
It's pretty surprising to me that this didn't require any changes to the Rust code in bootstrap -- can you at least briefly explain why? |
As best I understand, it's because the Rust code gets the build directory from bootstrap.py, and that was the case even before this PR. |
If you like I can try to run a full |
No, that's sufficient, I had forgotten about our custom logic there. Could you add a comment to that effect on the field added to the Build struct? |
Sure, I added a comment. |
Please update the PR description to reflect the current implementation and squash the commits into one; once that's done I think we should be able to land this. |
If the old default defined at rust/src/bootstrap/bootstrap.py Line 335 in 7ebd87a
|
This allows configuring the directory for build artifacts, instead of having it always be ./build. This means you can set it to a constant location, letting you reuse the same cache while working in several different directories. The configuration lives in config.toml under build.build-dir. By default, it keeps the existing default of ./build, but it can be configured to any relative or absolute path. Additionally, it allows making outputs relative to the root of the git repository using $ROOT.
Squashed the commits and removed the unused default |
Thanks! @bors r+ |
📌 Commit df36ec0 has been approved by |
@bors p=1 |
☀️ Test successful - checks-actions, checks-azure |
This allows configuring the directory for build artifacts, instead of having it always be
./build
. This means you can set it to a constant location, letting you reuse the same cache while working in several different directories.The configuration lives in
config.toml
underbuild.build-dir
. By default, it keeps the existing default of./build
, but it can be configured to any relative or absolute path. Additionally, it allows making outputs relative to the root of the git repository using$ROOT
.r? @Mark-Simulacrum